refactor(spu): fold weaver-embedding into weaver-spu (PR-0.5.D)#277
Conversation
Per docs/specs/weaver-spu-Spec.md §10 PR-0.5.D. Mechanical refactor;
no behavior change. Moves weaver-embedding's encoder-side modules
(late_chunk, gRPC client, GGUF backend, cohort-pin guard, proto
generation) under weaver-spu. weaver-embedding becomes a deprecated
re-export shell so existing consumers compile unchanged during the
migration window (removed in PR-0.5.E).
Files moved (git mv preserves history):
weaver-embedding/src/ weaver-spu/src/
───────────────────── ───────────────
late_chunk.rs → encoder/late_chunk.rs
gguf_backend.rs → encoder/gguf_backend.rs (gated `gguf`)
grpc_client.rs → encoder/grpc_client_legacy.rs
(renamed; not feature-gated since
this is the production embedder
backend during the migration
window — talks to Python
weaver-embedder.service)
pin.rs → core/pin.rs
build.rs → merged into weaver-spu/build.rs
(compiles persephone proto
unconditionally now since
grpc_client_legacy is always
compiled)
tests/grpc_client.rs → tests/grpc_client_legacy.rs
Removed (already moved in PR-0.5.B):
embedder.rs (deprecated re-export of weaver-core::embedder; the
re-exports moved to weaver-embedding/src/lib.rs)
weaver-embedding becomes a re-export shell:
- src/lib.rs: deprecated `pub use weaver_core::embedder::*` (Embedder
trait + EmbedderInfo) and deprecated `pub use weaver_spu::*`
(late_chunk, pin, grpc_client, gguf_backend, proto). The
Embedder trait stays as `pub use` (no stable trait-alias);
data types use deprecated `pub type` aliases for reliable
consumer-side warnings (matches PR-0.5.B's pattern).
- Cargo.toml: dropped 8+ transitive deps (tonic, prost, hyper-util,
tower, async-trait, chrono, sha2, llama-cpp-2, tokenizers,
tempfile, candle-*, etc.). Sole runtime deps now are
weaver-core (for the trait) and weaver-spu (with
default-features = false to forward features explicitly).
weaver-spu Cargo.toml gains:
- tonic, prost, prost-types, hyper-util, tower, async-trait,
serde_json, chrono, sha2 (encoder gRPC stack)
- tonic-build (build-dep, for proto compile)
- tokio-stream (dev-dep, for grpc_client_legacy tests' tonic
server fixture)
build.rs merged: combines the legacy CUDA-kernel compile (gated by
`cuda` feature, folded in PR-0.5.C) with the persephone proto
compile (always compiled now, since the gRPC client is the
production embedder backend during the migration window).
Internal imports rewritten:
- crate::embedder::* in grpc_client_legacy (referencing the trait
+ types when those still lived in weaver-embedding) →
weaver_core::embedder::* (the trait's new home post-PR-0.5.B).
Acceptance criteria from spec §10 PR-0.5.D:
- cargo build --workspace --features gguf,embedder clean ✓
- cargo test --workspace clean — all existing embedder tests pass ✓
(110 in weaver-embedding's deprecated shell + 12 ensure_ready
tests in the moved grpc_client_legacy.rs integration test
file + 588 in weaver-core)
- Existing Python embedder still operational via grpc_client_legacy ✓
Dependencies: PR-0.5.A (skeleton), PR-0.5.B (Embedder trait
relocation), PR-0.5.C (weaver-inference fold).
Companion upstream PR: none.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR converts crates/weaver-embedding into a deprecated re-export shell (forwarding public APIs to weaver-spu and weaver-core), removes its proto build and many backend dependencies, moves proto compilation and the gRPC/protobuf surface into weaver-spu, updates weaver-spu to compile the embedding proto, and adjusts tests and trait impls to the new locations. Changesweaver-embedding → weaver-spu / weaver-core Consolidation
Sequence DiagramsequenceDiagram
participant Dev as Developer (cargo build/test)
participant Build as weaver-spu build.rs
participant Proto as Proto files (proto/persephone/embedding)
participant SPU as crates::weaver_spu (compiled proto + API)
participant EMB as crates::weaver-embedding (deprecated shell)
participant Client as Tests / EmbeddingClient
Dev->>Build: cargo build (weaver-spu)
Build->>Proto: locate & compile persephone/embedding.proto
Build->>SPU: generate tonic bindings (server+client)
SPU->>EMB: EMB re-exports point at SPU/weaver-core
Client->>SPU: use weaver_spu::proto::embedding and grpc_client_legacy
Client->>EMB: (deprecated) follow re-exports -> SPU/weaver-core
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-embedding/src/lib.rs`:
- Around line 10-18: Update the crate-level docs to match the actual exports:
either remove the incorrect "gated by `gguf`" notes for the gRPC client and
proto lines or gate the actual exported items behind cfg(feature = "gguf").
Specifically reconcile the doc lines referencing
weaver_spu::encoder::grpc_client_legacy and weaver_spu::proto::embedding::*
(which mention EmbeddingClient, EmbeddingClientConfig, EmbeddingEndpoint) with
the real exports around the
EmbeddingClient/EmbeddingClientConfig/EmbeddingEndpoint symbols; ensure docs
only claim a feature gate when the corresponding modules/exports are annotated
with #[cfg(feature = "gguf")] (or conversely, add that attribute to the
module/exports if you intend them to be gated).
In `@crates/weaver-spu/build.rs`:
- Around line 72-83: The build script currently emits a single
cargo:rerun-if-changed for proto_root but not for each proto file; update the
code that builds the protos (the protos Vec and the tonic_build::configure()
block) to also emit cargo:rerun-if-changed lines for every path in protos by
iterating over protos and calling println!("cargo:rerun-if-changed={}",
proto.display()) (or equivalent) for each proto so Cargo will re-run the build
when any individual .proto changes.
- Around line 5-11: Update the top-of-file doc comment in build.rs to reflect
that the Persephone embedding-service proto compile step is not gated by the
gguf feature; state that it is always compiled because the production embedder
backend grpc_client_legacy requires those generated proto types. Specifically
adjust the sentences that claim the step is "gated by `gguf` feature" (mentions
of Persephone embedding-service and `gguf`) to instead note that proto
generation is unconditional due to `grpc_client_legacy`.
In `@crates/weaver-spu/Cargo.toml`:
- Around line 47-59: The comment above the gRPC dependency block is misleading:
it says "Active only when the `gguf` feature is set" but the listed crates
(tonic, prost, prost-types, hyper-util, tower, async-trait, serde_json, chrono,
sha2) are not optional and are always compiled; update the comment in
crates/weaver-spu/Cargo.toml to reflect that these dependencies are
unconditional because the encoder::grpc_client_legacy embedder backend is a
production backend and is always built (remove or change the `gguf` feature
gating text and mention that grpc_client_legacy is always active).
In `@crates/weaver-spu/src/encoder/mod.rs`:
- Around line 9-14: The doc comment incorrectly states that grpc_client_legacy
is feature-gated; update the module documentation to remove "feature-gated
`gguf`" for grpc_client_legacy and instead describe it as the non-feature-gated
production embedder backend used during the migration window (while keeping
gguf_backend described as feature-gated if applicable), ensuring the text
matches the actual module declaration for grpc_client_legacy and references both
grpc_client_legacy and gguf_backend by name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87a0e5eb-ad9f-4823-8220-b20fb8098db6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/weaver-embedding/Cargo.tomlcrates/weaver-embedding/build.rscrates/weaver-embedding/src/embedder.rscrates/weaver-embedding/src/lib.rscrates/weaver-spu/Cargo.tomlcrates/weaver-spu/build.rscrates/weaver-spu/src/core/mod.rscrates/weaver-spu/src/core/pin.rscrates/weaver-spu/src/encoder/gguf_backend.rscrates/weaver-spu/src/encoder/grpc_client_legacy.rscrates/weaver-spu/src/encoder/late_chunk.rscrates/weaver-spu/src/encoder/mod.rscrates/weaver-spu/src/lib.rscrates/weaver-spu/tests/grpc_client_legacy.rs
💤 Files with no reviewable changes (2)
- crates/weaver-embedding/src/embedder.rs
- crates/weaver-embedding/build.rs
Five CR findings on PR #277 commit edaa11a, all valid. The code correctly un-gated `encoder::grpc_client_legacy` and the `proto` module from the `gguf` feature (since the legacy gRPC client is the production embedder backend during the migration window, not a feature-gated path). But several doc / comment sites still described them as feature-gated. Doc-only fixes: 1. `weaver-embedding/src/lib.rs` lines 14-23: corrected the new-code import guide. gRPC client + proto noted as "always available", not "gated by gguf". 2. `weaver-spu/build.rs` top-of-file doc: corrected the proto step description — "always compiled because grpc_client_legacy depends on the generated types unconditionally", not gated. 3. `weaver-spu/build.rs` proto compile: added per-proto `cargo:rerun-if-changed` lines (in addition to the proto_root one) so editing any individual `.proto` triggers a rebuild reliably across cargo versions. 4. `weaver-spu/Cargo.toml` encoder gRPC stack comment: replaced "Active only when the `gguf` feature is set" with the correct "Always compiled — grpc_client_legacy is the production embedder backend today, not a feature-gated path." 5. `weaver-spu/src/encoder/mod.rs` doc comment: reordered to put `grpc_client_legacy` first (as the production backend, not feature-gated) and clarified `gguf_backend` as the experimental in-process path that's the actual feature-gated module. Validated: cargo check --workspace clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-spu/src/encoder/mod.rs`:
- Around line 20-28: The top-level module documentation in
crates::weaver_spu::encoder currently advertises future modules (lora_dispatch,
pooling, matryoshka, jina_v4, client) that do not exist; update the doc comment
in mod.rs to avoid exposing non-existent APIs by either removing those bullet
items or marking them explicitly as "planned" or "future work" (e.g., change the
bullets to a "Planned modules" section or delete them until implemented). Edit
the doc block containing the listed module names near the existing late_chunk,
grpc_client_legacy, and gguf_backend mentions so rustdoc only documents actual,
importable modules or clearly labels the others as forthcoming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba13c454-110e-40a9-a19c-675f56ba3a7d
📒 Files selected for processing (4)
crates/weaver-embedding/src/lib.rscrates/weaver-spu/Cargo.tomlcrates/weaver-spu/build.rscrates/weaver-spu/src/encoder/mod.rs
Per CR finding on 5e29bcf: the encoder/mod.rs doc listed `lora_dispatch`, `pooling`, `matryoshka`, `jina_v4`, `client` as modules without flagging that they don't exist yet. Renamed the section "Planned modules (Phase 1 — not yet present)" with explicit "planned" labels per module, plus a sentence noting the docs are intentionally there so reviewers can spot drift between the planned surface and what lands during Phase 1 PRs.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Per
docs/specs/weaver-spu-Spec.md§10 PR-0.5.D. Mechanical refactor; no behavior change. Movesweaver-embedding's encoder-side modules (late_chunk, gRPC client, GGUF backend, cohort-pin guard, proto generation) underweaver-spu.weaver-embeddingbecomes a deprecated re-export shell so existing consumers compile unchanged during the migration window (removed in PR-0.5.E).File moves (git mv preserves history)
weaver-embedding/src/weaver-spu/src/late_chunk.rsencoder/late_chunk.rsgguf_backend.rsencoder/gguf_backend.rs(gatedgguf)grpc_client.rsencoder/grpc_client_legacy.rs(renamed; not feature-gated since this is the production embedder backend during the migration window — talks to Pythonweaver-embedder.service)pin.rscore/pin.rsbuild.rsweaver-spu/build.rstests/grpc_client.rstests/grpc_client_legacy.rsRemoved (already replaced by PR-0.5.B):
embedder.rs(the re-exports moved tolib.rs).weaver-embedding becomes a re-export shell
src/lib.rs: deprecatedpub use weaver_core::embedder::*(Embedder trait + EmbedderInfo) and deprecatedpub use weaver_spu::*(late_chunk, pin, grpc_client, gguf_backend, proto). Trait stays aspub use(no stable trait-alias); data types use deprecatedpub typealiases for reliable consumer-side warnings (matches PR-0.5.B pattern).Cargo.toml: dropped 8+ transitive deps (tonic, prost, hyper-util, tower, async-trait, chrono, sha2, llama-cpp-2, tokenizers, tempfile, candle-*, etc.). Sole runtime deps now areweaver-coreandweaver-spu(withdefault-features = falseto forward features explicitly viagguf).weaver-spu Cargo.toml gains
tonic,prost,prost-types,hyper-util,tower,async-trait,serde_json,chrono,sha2(encoder gRPC stack)tonic-build(build-dep, for proto compile)tokio-stream(dev-dep, forgrpc_client_legacytests' tonic server fixture)build.rs merged
Combines:
cudafeature, folded in PR-0.5.C)Test plan
cargo build --workspace --features gguf,embeddercleancargo test --workspaceclean — all existing tests pass (110 weaver-embedding shell + 12 ensure_ready tests in moved grpc_client_legacy.rs + 588 weaver-core)grpc_client_legacySequencing
🤖 Generated with Claude Code
Summary by CodeRabbit
Deprecations
Chores
Tests